Trivy Scanning tool pilot implementation #72
Conversation
tmikula-dev
requested review from
ABMC831,
Zejnilovic,
oto-macenauer-absa and
petr-pokorny-absa
as code owners
WalkthroughRefactors CI into detect-first workflows for Terraform, Docker, and Python; adds full-repo Trivy scan with SARIF parsing and PR comment; introduces Trivy secret rules; removes legacy multi-job Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Detect as Detect Job
participant Analyzer as Conditional Jobs
participant Scanner as Tools (Trivy / TFLint / Pylint / Black / Pytest / Mypy)
participant SARIF as SARIF Upload & Parser
participant Noop as Noop Job
GH->>Detect: trigger (push/pr/workflow_dispatch)
Detect->>Detect: compute git-diff range\nset <lang>_changed output
alt changed == true
Detect-->>Analyzer: changed=true
GH->>Analyzer: run conditional jobs
Analyzer->>Scanner: run tools
opt SARIF-producing
Scanner->>SARIF: produce SARIF
SARIF->>GH: upload SARIF
SARIF->>GH: parse SARIF -> generate summary table
SARIF->>GH: (PR) post comment with summary
end
else changed == false
Detect-->>Noop: changed=false
GH->>Noop: run noop (log minimal message)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/check_terraform.yml (1)
19-19: Minor: Clarify the comment explaining workflow intent.The comment "This job is implemented this way, because we need workflow mandatory" is unclear. Consider rephrasing to explain that the detect job ensures the workflow always executes (even with no changes) to maintain mandatory status in branch protection rules.
- ## This job is implemented this way, because we need workflow mandatory + ## Detect job ensures workflow runs regardless of changes, making it mandatory in branch protection rules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/check_terraform.yml(3 hunks)
🔇 Additional comments (4)
.github/workflows/check_terraform.yml (4)
45-46: Conditional execution structure looks good.The
needs: detectdependency andif: needs.detect.outputs.changed == 'true'conditions correctly gate the trivy and tflint jobs, ensuring they skip when terraform/ has no changes while maintaining overall workflow status via the noop job.Also applies to: 83-84
122-127: Noop job effectively preserves mandatory workflow status.The noop job ensures at least one job always completes successfully, preventing the workflow from being skipped entirely when terraform/ hasn't changed—a critical design pattern for making this workflow mandatory in branch protection.
25-25: Consistent checkout action upgrade to v5.All three jobs (detect, trivy, tflint) uniformly upgrade to
actions/checkout@v5while preserving thefetch-depth: 0configuration needed for git history operations.Also applies to: 49-49, 87-87
32-42: I'll verify the critical claim about the git diff edge case and GitHub Actions behavior.Let me verify one more critical detail about how
HEAD~1...HEADbehaves on the first commit.Critical issue is valid, but one suggested fix is flawed. The
HEAD~1...HEADapproach does not gracefully handle first commits—it also fails with a fatal error.Using
git diff HEAD^ HEADon the first commit results in "fatal: ambiguous argument 'HEAD^': unknown revision or path not in the working tree". The same applies toHEAD~1...HEAD. The review's claim that this approach "handles the initial commit gracefully" is incorrect.The conditional check approach (using
git rev-parse "${{ github.sha }}~1"to verify the parent exists) remains the correct solution. Remove the misleading suggestion aboutHEAD~1...HEAD.Likely an incorrect or invalid review comment.
tmikula-dev
added
the
work in progress
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Dockerfile (3)
21-26: Critical: Docker build fails due to missing COPY source paths.The pipeline failures show COPY commands referencing build args that don't exist by default:
./trusted_certsand./sasl_ssl_artifacts. When these directories are not provided at build time, the COPY operations fail, breaking the Docker image build.Either provide default directories that exist in the repository, make these COPY operations conditional with fallbacks, or document that these build args are required and must be supplied during builds.
Suggested fix: Make COPY operations conditional or require the build args:
# Option 1: Make paths optional by checking if they exist ARG TRUSTED_SSL_CERTS=./trusted_certs ARG SASL_SSL_ARTIFACTS=./sasl_ssl_artifacts # Create empty directories as fallbacks RUN mkdir -p /opt/certs/ /opt/sasl_ssl_artifacts/ # Conditionally copy only if directories exist and have files COPY --chown=0:0 ${TRUSTED_SSL_CERTS}/*.pem /opt/certs/ 2>/dev/null || true COPY ${SASL_SSL_ARTIFACTS} /opt/sasl_ssl_artifacts/ 2>/dev/null || trueOr:
# Option 2: Require the build args and ensure they're provided ARG TRUSTED_SSL_CERTS ARG SASL_SSL_ARTIFACTS COPY ${TRUSTED_SSL_CERTS} /opt/certs/ COPY ${SASL_SSL_ARTIFACTS} /opt/sasl_ssl_artifacts/
18-18: Minor: FROM --platform flag uses constant value.Hadolint warns against using constant platform flags in FROM statements, as this limits portability and should typically be left to runtime configuration or build context.
This is a low-priority warning but can be addressed by removing the platform specification if cross-platform builds aren't a requirement, or documenting the ARM64-specific constraint.
66-66: Verify LDFLAGS path for librdkafka.Line 66 sets
LDFLAGS="-L/opt", but line 53 installs librdkafka to the default location (likely/usr/local/lib). The LDFLAGS path should match the installation directory:- CPPFLAGS="-I/usr/local/include" LDFLAGS="-L/opt" python setup.py install + CPPFLAGS="-I/usr/local/include" LDFLAGS="-L/usr/local/lib" python setup.py install
🧹 Nitpick comments (3)
Dockerfile (1)
50-53: Consider adding checksums for downloaded binaries.Lines 50 and 63 download source archives without verifying integrity. Adding checksums improves security and reproducibility:
RUN \ mkdir -p /tmp/env-install-workdir/librdkafka && \ cd /tmp/env-install-workdir/librdkafka && \ wget https://github.com/edenhill/librdkafka/archive/v2.4.0.tar.gz && \ echo "expected_sha256_hash v2.4.0.tar.gz" | sha256sum -c - && \ tar -xf v2.4.0.tar.gz && \ # ... rest of buildAlso applies to: 63-66
.github/workflows/check_python.yml (1)
35-44: Change detection logic is sound but consider edge cases.The detect job correctly uses git diff with proper branching logic for PRs vs pushes. However, the pattern
^(src|tests)/matches files at any path depth; ensure this covers nested directories likesrc/module/file.py(which it does).One edge case: If only configuration files like
setup.cfgorpytest.inichange, Python analysis still runs. Consider whether config-only changes should trigger the workflow..github/workflows/check_docker.yml (1)
40-40: Dockerfile detection pattern matches only root-level Dockerfile.Line 40 uses pattern
^Dockerfile$, which only matches the Dockerfile at the repository root. If other Dockerfiles exist in subdirectories (e.g.,services/api/Dockerfile), they won't trigger this workflow.If the repo has multiple Dockerfiles, update the pattern:
if git diff --name-only "$RANGE" | grep -qE '(^|/)Dockerfile'; thenOtherwise, document that only the root Dockerfile is scanned.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/check_docker.yml(1 hunks).github/workflows/check_python.yml(1 hunks).github/workflows/check_terraform.yml(2 hunks).github/workflows/test.yml(0 hunks)DEVELOPER.md(1 hunks)Dockerfile(1 hunks)README.md(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/test.yml
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- DEVELOPER.md
🧰 Additional context used
🪛 GitHub Actions: Static Docker Check
Dockerfile
[error] 5-5: failed to calculate checksum of ref ac1187db-1850-4397-a0f7-3b6e6ef293d6::/trusted_certs: "/trusted_certs" not found
[error] 7-7: failed to calculate checksum of ref ac1187db-1850-4397-a0f7-3b6e6ef293d6::/sasl_ssl_artifacts: not found
[warning] 18-18: FromPlatformFlagConstDisallowed: FROM --platform flag should not use constant value "linux/arm64"
[error] 74-74: COPY $SASL_SSL_ARTIFACTS /opt/sasl_ssl_artifacts/ exited with error due to missing source path
🔇 Additional comments (5)
.github/workflows/check_python.yml (2)
127-128: Pytest coverage threshold at 80% is strict—verify it's achievable.The
--cov-fail-under=80flag enforces 80% coverage on tests. Ensure the test suite actually meets this threshold; if tests fail due to coverage gaps, CI will block merges.Consider running locally to verify coverage:
pytest --cov=. -v tests/ --cov-fail-under=80
103-105: This review comment is incorrect and should be dismissed.The workflow's approach is intentional and documented. The
detectjob gates all downstream analysis jobs to run only when Python files change (lines 48-49:if: needs.detect.outputs.python_changed == 'true'). When Python changes are detected, validating the full codebase—not just diffs—is the documented, recommended practice per DEVELOPER.md, which explicitly instructs developers to run checks on "all files currently tracked by Git" usingblack $(git ls-files '*.py')andpylint $(git ls-files '*.py').This two-tier design (detect → validate full consistency) is sound: it avoids redundant checks when no Python changes exist while ensuring codebase-wide quality standards when changes occur.
Likely an incorrect or invalid review comment.
.github/workflows/check_terraform.yml (3)
35-44: Terraform change detection aligns with Python and Docker workflows.The detect job uses identical logic and structure to the Python and Docker workflows, which is excellent for consistency. The pattern
^terraform/correctly identifies changes in the terraform directory.
91-97: Trivy scan from terraform/ subdirectory is appropriate.The Trivy scan at line 94 runs from the terraform/ working directory, scanning the current directory (
.). This is correct and aligns with the Python workflow's approach of scanning only the relevant code paths.
48-49: I need to gather more specific information about TFLint's version history and when SARIF support was introduced.TFLint SARIF support is well-established; no action needed.
SARIF format support was added to TFLint in v0.35.0, and the CLI currently supports
-f sarifas a documented output format option. When usingterraform-linters/setup-tflintwithtflint_version: latest, the action retrieves the latest TFLint version, which includes stable SARIF support. The concern about version-dependent SARIF availability is not applicable—TFLint's 0.x version scheme does not have a v1.40, and SARIF has been a standard, supported output format for years without breaking changes.
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
Please look at the suggested solution. This PILOT has a team goal to understand and set the standard of working with workflows and scanners inside our projects. I did follow the splitting the workflows for determined purposes. Open to the discussion. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/trivy_repository_scan.yml (2)
97-108: Minor: Inconsistent error handling for missing GITHUB_OUTPUT.When
GITHUB_OUTPUTis not set (line 104), the script prints a warning but continues with exit code 0, which means the downstream comment step will receive an empty summary table. For consistency with the IOError handling (line 108), either exit with code 1 here or handle the fallback more explicitly. In practice,GITHUB_OUTPUTis always set in GitHub Actions environments, so this is a defensive edge case.Consider either treating missing
GITHUB_OUTPUTas a fatal error:else: - print("Warning: GITHUB_OUTPUT not set", file=sys.stderr) + raise RuntimeError("GITHUB_OUTPUT environment variable not set")Or explicitly validate before writing. Either approach makes the error path consistent with the
IOErrorhandler below.
25-31: Confirm scanner scope aligns with pilot objectives.The workflow includes four scanners:
vuln,secret,misconfig, andlicense. Given this is a pilot to establish standards across projects, verify this scanner configuration matches your intended scope. For example, some teams may want to exclude license scanning or add additional scanners. Consider documenting the rationale in the repository's security scanning guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/trivy_repository_scan.yml(1 hunks)
🔇 Additional comments (3)
.github/workflows/trivy_repository_scan.yml (3)
111-126: Conditional check properly addresses PR context requirement.The
if: github.event_name == 'pull_request'condition on line 112 correctly gates the PR comment step, ensuring it only runs when triggered by a pull request event, not manualworkflow_dispatchruns. This addresses the past critical issue and is the appropriate solution.
3-6: Confirm dual trigger approach aligns with team standards.The workflow triggers on both
workflow_dispatch(manual) andpull_requestevents. While this provides flexibility—allowing both automatic PR scanning and on-demand manual scans—confirm this dual-trigger pattern aligns with the standards the pilot intends to establish. For reference, the past review approved the conditional gating of the PR comment step (line 112), which handles this dual-trigger scenario correctly.
25-26: No action required — version is current.The aquasecurity/setup-trivy action version v0.2.4 is the latest version, so the pinned version used in the workflow is appropriate and follows best practices for maintaining reproducible CI/CD pipelines.
| - name: Create scan summary table | ||
| id: scan_summary_table | ||
| run: | | ||
| python <<'PY' | ||
| import os | ||
| import json | ||
| import sys | ||
| from collections import Counter | ||
| SARIF_PATH = "trivy_repository_report.sarif" | ||
| SEVERITIES = ["CRITICAL", "HIGH", "MEDIUM", "LOW"] | ||
| try: | ||
| # Parse results from SARIF | ||
| with open(SARIF_PATH, "r", encoding="utf-8") as f: | ||
| sarif = json.load(f) | ||
| # Validate SARIF structure | ||
| if "runs" not in sarif or not sarif["runs"]: | ||
| raise ValueError("SARIF file contains no runs") | ||
| run = sarif["runs"][0] | ||
| if "tool" not in run or "driver" not in run["tool"]: | ||
| raise ValueError("SARIF structure missing expected tool/driver keys") | ||
| rules = run["tool"]["driver"].get("rules", []) | ||
| results = run.get("results", []) | ||
| counts = Counter() | ||
| except (IOError, json.JSONDecodeError, KeyError, ValueError) as e: | ||
| print(f"Error processing SARIF file: {e}", file=sys.stderr) | ||
| sys.exit(1) | ||
| # Count results by severity | ||
| for res in results: | ||
| try: | ||
| rule_idx = res.get("ruleIndex") | ||
| if rule_idx is None or rule_idx >= len(rules): | ||
| continue | ||
| rule = rules[rule_idx] | ||
| tags = rule.get("properties", {}).get("tags", []) | ||
| sev = next((s for s in SEVERITIES if s in tags), None) | ||
| if sev: | ||
| counts[sev] += 1 | ||
| except (KeyError, IndexError, TypeError) as e: | ||
| print(f"Warning: Error processing result: {e}", file=sys.stderr) | ||
| continue | ||
| headers = SEVERITIES + ["TOTAL"] | ||
| values = [str(counts.get(s, 0)) for s in SEVERITIES] | ||
| total = sum(counts.get(s, 0) for s in SEVERITIES) | ||
| values.append(str(total)) | ||
| # Build Markdown summary table | ||
| summary_table = "| " + " | ".join(headers) + " |\n" | ||
| summary_table += "|" + "|".join(["---"] * len(headers)) + "|\n" | ||
| summary_table += "| " + " | ".join(values) + " |" | ||
|
|
||
| # Set output | ||
| try: | ||
| if "GITHUB_OUTPUT" in os.environ: | ||
| with open(os.environ["GITHUB_OUTPUT"], "a", encoding="utf-8") as f: | ||
| f.write("table<<EOF\n") | ||
| f.write(summary_table + "\n") | ||
| f.write("EOF\n") | ||
| else: | ||
| print("Warning: GITHUB_OUTPUT not set", file=sys.stderr) | ||
|
|
||
| except IOError as e: | ||
| print(f"Error writing output: {e}", file=sys.stderr) | ||
| sys.exit(1) | ||
| PY |
There was a problem hiding this comment.
🧩 Analysis chain
Verify SARIF structure assumptions for severity extraction.
The Python script assumes severity information is located in rule.properties.tags, but this depends on Trivy's specific SARIF output structure. Verify that Trivy places severity information in this exact location; if severity is instead a property of the result or in a different SARIF field, the severity counts will be incorrect.
You can verify this by running Trivy and inspecting the generated trivy_repository_report.sarif file to confirm the structure matches the script's expectations:
🏁 Script executed:
#!/bin/bash
# Generate sample SARIF to inspect structure
trivy fs --format sarif --output /tmp/sample.sarif . 2>/dev/null || true
# Display relevant portions of the SARIF structure
echo "=== Sample rule structure ==="
jq '.runs[0].tool.driver.rules[0] | {id, shortDescription, properties}' /tmp/sample.sarif 2>/dev/null || echo "File not generated"
echo "=== Sample result structure ==="
jq '.runs[0].results[0] | {ruleId, ruleIndex, message, properties}' /tmp/sample.sarif 2>/dev/null || echo "No results in sample"Length of output: 101
Let me search more specifically for Trivy's SARIF structure with examples:
Verify SARIF structure assumptions for severity extraction with actual Trivy output or source code inspection.
The Python script assumes severity is in rule.properties.tags based on Trivy's SARIF implementation which maps severity levels using the toSarifErrorLevel function, but the exact SARIF structure cannot be definitively confirmed through web search alone. Without access to an actual Trivy SARIF output file or the repository's Trivy source code examination, the severity extraction approach in the script remains unverified.
To confirm correctness, inspect a real SARIF file from Trivy (e.g., in the repository's test data or CI outputs) to verify the location of severity information matches the script's expectations of rules[ruleIndex].properties.tags.
🌐 Web query:
Trivy SARIF output format severity field location
💡 Result:
Short answer — where Trivy puts severity in SARIF:
- Trivy maps its severity to the SARIF result "level" (error/warning/note/none) for each result. [1][2]
- Trivy also embeds severity metadata in the SARIF properties (e.g., properties.tags includes the Trivy severity string and properties["security-severity"] holds a numeric CVSS-like score); rule/defaultConfiguration.level and rule.properties may also be populated. [1][2]
- Consumers that follow the SARIF 2.1.0 spec should read result.level first, and can consult result.properties / rule.defaultConfiguration.level / rule.properties for Trivy-specific severity details. [3]
Sources:
[1] Trivy source (pkg/report/sarif.go) — mapping and properties.
[2] Trivy SARIF documentation / examples.
[3] SARIF v2.1.0 specification (result.level and rule.defaultConfiguration.level).
Fix severity extraction to use SARIF result.level as primary source.
The Python script extracts severity only from rule.properties.tags, but according to SARIF 2.1.0 specification and Trivy's implementation, the primary severity location is result.level. The script should read result.level first, with fallback to result.properties.tags or result.properties["security-severity"] for additional context. The current approach bypasses the standard SARIF severity field and relies on an indirect lookup through rules, which may miss severities or fail if ruleIndex is incorrect.
Update lines 72–81 to check result.level or result.properties as the primary source:
# Count results by severity
for res in results:
try:
# Check result.level (primary per SARIF 2.1.0) or result.properties.tags
sev = None
if "level" in res:
level_map = {"error": "CRITICAL", "warning": "HIGH", "note": "MEDIUM", "none": "LOW"}
sev = level_map.get(res["level"])
if not sev:
# Fallback to properties.tags if level not found
tags = res.get("properties", {}).get("tags", [])
sev = next((s for s in SEVERITIES if s in tags), None)
if sev:
counts[sev] += 1
except (KeyError, TypeError) as e:
print(f"Warning: Error processing result: {e}", file=sys.stderr)
continue
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/trivy_repository_scan.yml(1 hunks)
🔇 Additional comments (6)
.github/workflows/trivy_repository_scan.yml (6)
3-12: Conditional logic for PR-only steps is properly gated.The workflow triggers on both
workflow_dispatchandpull_request, and the PR comment step at line 130 correctly usesif: github.event_name == 'pull_request'to skip when triggered manually. This properly addresses the past concern about context availability.
28-33: Trivy command properly generates SARIF output file.The command correctly specifies
--format sarifand outputs totrivy_repository_report.sarif, which addresses the past critical issue and enables the downstream SARIF upload and summary parsing steps to function correctly.
92-112: Table generation and aggregation logic is sound.The Markdown table structure is correct with proper column alignment, and the aggregation logic properly counts results by category and severity across all rows.
115-126: GITHUB_OUTPUT writing uses safe heredoc pattern with proper validation.The implementation correctly checks for GITHUB_OUTPUT environment variable and uses the heredoc pattern (EOF delimiter) for safe multi-line output. Error handling is appropriate.
53-72: SARIF validation is comprehensive with clear error messages.The SARIF file loading includes defensive checks for required structure (runs, tool, driver keys) and handles parsing errors gracefully with informative exit messages.
129-143: PR comment step is properly conditional and constructs informative comment.The conditional
if: github.event_name == 'pull_request'prevents failures on manual dispatch. The comment body includes a direct link to the security overview and embeds the summary table from the previous step.
| for res in results: | ||
| try: | ||
| rule_idx = res.get("ruleIndex") | ||
| if rule_idx is None or rule_idx >= len(rules): | ||
| continue | ||
| rule = rules[rule_idx] | ||
| tags = rule.get("properties", {}).get("tags", []) | ||
| # Find category and severity | ||
| category = next((c for c in CATEGORIES if c in tags), None) | ||
| severity = next((s for s in SEVERITIES if s in tags), None) | ||
| if category and severity: | ||
| category_severity_counts[category][severity] += 1 | ||
| except (KeyError, IndexError, TypeError) as e: | ||
| print(f"Warning: Error processing result: {e}", file=sys.stderr) | ||
| continue |
There was a problem hiding this comment.
Critical: Use SARIF standard result.level field for severity extraction.
The current code extracts severity only from rule.properties.tags, bypassing the SARIF 2.1.0 specification's primary severity field (result.level). This is fragile and non-standard. Per Trivy's implementation, result.level (error/warning/note/none) should be the primary severity source, with fallback to tags or properties only if missing.
This issue was flagged in a past review but remains unaddressed.
Apply this diff to check result.level first:
# Count results by category and severity
for res in results:
try:
# Extract category from tags (required)
tags = res.get("properties", {}).get("tags", [])
category = next((c for c in CATEGORIES if c in tags), None)
if not category:
continue
# Extract severity: check result.level (primary per SARIF spec)
severity = None
if "level" in res:
level_map = {"error": "CRITICAL", "warning": "HIGH", "note": "MEDIUM", "none": "LOW"}
severity = level_map.get(res["level"])
# Fallback to tags if result.level not available
if not severity:
severity = next((s for s in SEVERITIES if s in tags), None)
if severity:
category_severity_counts[category][severity] += 1
except (KeyError, TypeError) as e:
print(f"Warning: Error processing result: {e}", file=sys.stderr)
continueThis approach follows the SARIF standard, uses Trivy's result.level as the authoritative severity source, and removes the fragile ruleIndex lookup dependency.
🤖 Prompt for AI Agents
.github/workflows/trivy_repository_scan.yml around lines 75-89: the code
currently derives severity only from rule.properties.tags and uses ruleIndex
lookups; update logic to treat SARIF result.level as the primary severity source
(map {"error":"CRITICAL","warning":"HIGH","note":"MEDIUM","none":"LOW"}), fall
back to extracting severity from tags if level is missing, and stop relying on
ruleIndex for severity (still use tags only to determine category and as
fallback for severity); ensure you handle missing fields safely and keep the
existing exception handling.
|
Trivy has completed a full security repository scan ✅ You can find the analysis results for this PR branch on this overview.
|
|
In the Trivy scan tool official documentation, are listed features, that are enabled only to AquaSec (via this link]). For example SAST, Malware Scanning, Sandbox scanning, Leaked secrets validation etc. This solution is the closest we can get to the official scanning. There is still option to create new secret regex rules to expand our secret scanning. I am also not sure, if this current version of the workflow should trigger only, when the PR is created or every time the PR is updated. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/trivy_repository_scan.yml(1 hunks)
🔇 Additional comments (3)
.github/workflows/trivy_repository_scan.yml (3)
3-6: ✓ PR context conditional correctly implemented.The workflow now properly handles both
workflow_dispatchandpull_requestevents via the conditional at line 129 (if: github.event_name == 'pull_request'), preventing failures when triggered manually.
28-33: ✓ SARIF output format correctly specified.The Trivy command now properly generates SARIF output with
--format sarif --output trivy_repository_report.sarif, enabling downstream SARIF upload and parsing steps.
128-143: ✓ PR comment step properly guarded and uses secure token handling.The conditional check at line 129 ensures the step runs only for pull requests, and
secrets.GITHUB_TOKENis correctly used instead of hardcoded credentials.
| # Count results by category and severity | ||
| for result in results: | ||
| try: | ||
| rule_idx = result.get("ruleIndex") | ||
| if rule_idx is None or rule_idx >= len(rules): | ||
| continue | ||
| rule = rules[rule_idx] | ||
| tags = rule.get("properties", {}).get("tags", []) | ||
| # Find category and severity | ||
| category = next((c for c in CATEGORIES if c in tags), None) | ||
| severity = next((s for s in SEVERITIES if s in tags), None) | ||
| if category and severity: | ||
| category_severity_counts[category][severity] += 1 | ||
| except (KeyError, IndexError, TypeError) as e: | ||
| print(f"Warning: Error processing result: {e}", file=sys.stderr) | ||
| continue |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Use SARIF standard result.level as primary severity source.
The severity extraction still bypasses the SARIF 2.1.0 specification by reading only from rule.properties.tags (line 81, 84). Per SARIF spec and Trivy's implementation, result.level (error/warning/note/none) is the authoritative severity field. The current approach is fragile, non-standard, and may produce incorrect severity counts. This was flagged in a prior review but remains unaddressed.
Apply this diff to prioritize result.level:
# Count results by category and severity
for result in results:
try:
# Extract category from result tags (required)
result_tags = result.get("properties", {}).get("tags", [])
category = next((c for c in CATEGORIES if c in result_tags), None)
if not category:
continue
# Extract severity: check result.level (primary per SARIF spec)
severity = None
if "level" in result:
level_map = {"error": "CRITICAL", "warning": "HIGH", "note": "MEDIUM", "none": "LOW"}
severity = level_map.get(result["level"])
# Fallback to tags if result.level not available
if not severity:
severity = next((s for s in SEVERITIES if s in result_tags), None)
if severity:
category_severity_counts[category][severity] += 1
except (KeyError, TypeError) as e:
print(f"Warning: Error processing result: {e}", file=sys.stderr)
continueThis approach:
- Uses
result.levelas the authoritative severity source - Falls back to tags only if
levelis missing - Removes the fragile
ruleIndexlookup dependency - Follows the SARIF 2.1.0 standard and Trivy's design
I'll verify this review comment by examining the actual file and confirming the SARIF specification details.
🏁 Script executed:
#!/bin/bash
# Examine the workflow file and surrounding context
cat ".github/workflows/trivy_repository_scan.yml" | head -150 | tail -90Length of output: 4251
🏁 Script executed:
#!/bin/bash
# Get the full script context to understand the data structures
cat ".github/workflows/trivy_repository_scan.yml"Length of output: 5602
🌐 Web query:
SARIF 2.1.0 specification result.level severity field
💡 Result:
Summary — result.level in SARIF 2.1.0
- Field: runs[].results[].level (optional). [1]
- Allowed values and meanings:
- "warning": rule was evaluated and a problem was found. [1]
- "error": rule was evaluated and a serious problem was found. [1]
- "note": rule was evaluated and a minor problem or opportunity to improve. [1]
- "none": “severity” does not apply because result.kind is not "fail". [1]
- Rules: if present it must be one of the above; level relates to result.kind (if kind is present and level ≠ "none", kind MUST be "fail"); if kind is absent it defaults to "fail". [1]
- Spec / reference: SARIF 2.1.0 (OASIS). [1][2]
Sources:
[1] OASIS — Static Analysis Results Interchange Format (SARIF) Version 2.1.0 (section 3.27.10 and related).
[2] SARIF project site / documentation (overview for v2.1.0).
---
🌐 Web query:
```web
Trivy SARIF output result.level severity field
💡 Result:
Trivy maps its severities to SARIF result.level as follows:
- CRITICAL, HIGH → "error". [1]
- MEDIUM → "warning". [1]
- LOW, UNKNOWN → "note". [1]
- other → "none". [1]
(Trivy can emit SARIF via --format sarif; see Trivy reporting docs.) [2]
Sources: [1] DeepWiki summary of Trivy SARIF mapping (references Trivy's sarif.go). [2] Trivy reporting / SARIF docs.
Correct the severity level mappings in the proposed fix.
The review identifies a valid concern—the code should use result.level as primary severity source per SARIF spec, which Trivy does populate. However, the suggested level_map contains incorrect mappings:
Trivy maps CRITICAL/HIGH to "error", MEDIUM to "warning", and LOW/UNKNOWN to "note". The proposed diff maps "warning" to "HIGH" and "note" to "MEDIUM", which contradicts Trivy's actual output.
Correct mappings should be:
"error"→"CRITICAL"(or"HIGH"—choose one, as Trivy conflates both)"warning"→"MEDIUM""note"→"LOW""none"→"LOW"(or skip)
Apply the suggested approach of using result.level as primary source, but fix the level_map conversions to match Trivy's actual severity mappings.
🤖 Prompt for AI Agents
.github/workflows/trivy_repository_scan.yml around lines 74-89: the code should
use result["level"] (SARIF/Trivy) as the primary severity source and the
level_map must be corrected to match Trivy: map "error" -> CRITICAL (or HIGH),
"warning" -> MEDIUM, "note" -> LOW, and treat "none" as LOW or skip; update the
logic to prefer result.get("level") and only fall back to tags-based mapping
when level is missing, then convert the Trivy level via the corrected level_map
before incrementing category_severity_counts.
|
Trivy has completed a full security repository scan ✅ You can find the analysis results for this PR branch on this overview.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/check_docker.yml (2)
62-66: Consider explicitly specifying Trivy scanners in the config scan.The
trivy config Dockerfilecommand uses Trivy's default scanners. For clarity and reproducibility, consider explicitly specifying which scanners to run (e.g.,--scanners misconfig,secret) to align with the PR's stated goal of standardizing scanner usage and to match the configuration in other workflows liketrivy_repository_scan.yml.- trivy config Dockerfile \ + trivy config Dockerfile \ + --scanners misconfig,secret \ --format sarif \ --output $GITHUB_WORKSPACE/trivy_dockerfile.sarif
68-71: No error handling if Trivy scan fails or SARIF file is missing.If the Trivy scan fails or produces no output, the SARIF upload step will still execute and may fail silently or produce unclear error messages. Consider adding a conditional to ensure the SARIF file exists before upload, or add explicit error handling.
- name: Upload Dockerfile SARIF + if: success() uses: github/codeql-action/upload-sarif@v4 with: sarif_file: ${{ github.workspace }}/trivy_dockerfile.sarif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/check_docker.yml(1 hunks).github/workflows/check_terraform.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/check_terraform.yml
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/check_docker.yml
1-1: workflow is empty
(syntax-check)
🔇 Additional comments (3)
.github/workflows/check_docker.yml (3)
1-79: Entire workflow is commented out; unclear if intentional for pilot phase or accidental.The entire workflow is disabled via comments. Line 1 references issue #74 for future enablement, and the PR description describes this as a "pilot implementation," suggesting this may be intentional. However, this creates a gap between the PR objectives (which claim to "introduce Trivy scanning") and the actual state (workflow is non-functional).
Clarify the intent: should this workflow remain commented out as part of the pilot, or should it be uncommented and enabled in this PR?
If keeping it commented out is intentional, consider adding a clear issue reference and timeline in the TODO comment so maintainers understand when to revisit this.
36-45: Git diff logic looks correct for detecting Dockerfile changes.The detect job properly handles both
pull_requestandpushevent types with appropriate git ranges, allowing the conditional jobs to gate ondocker_changedoutput. This pattern is sound.
1-1: Previous review flagged Docker build failure; verify applicability to this workflow.A previous review comment noted: "The docker build at line 62 will fail due to the Dockerfile issues identified in the Dockerfile review (missing COPY source paths)."
However, this workflow does not contain a
docker buildstep—it only runstrivy config Dockerfileto scan Dockerfile syntax. This past comment appears to reference a different workflow or an earlier version of this file.Please verify whether the Docker build issue is:
- Already resolved in the repository (missing
trusted_certs/andsasl_ssl_artifacts/directories added), or- Addressed elsewhere (e.g., in a separate Docker build workflow not shown here), or
- Not applicable to this workflow.
If a separate Docker image build and scan is needed, consider creating a distinct
trivy imagejob (building an image from the Dockerfile and scanning it), separate from the config scan shown here.
Release Notes:
Summary by CodeRabbit
New Features
Improvements
Documentation
Maintenance